Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Glibc module: detect location of glibc header files #282

Merged
merged 1 commit into from
Dec 10, 2015

Conversation

orobio
Copy link
Contributor

@orobio orobio commented Dec 6, 2015

In the Glibc module map all sys/... header files had the following paths:
/usr/include/x86_64-linux-gnu/sys/..., but some distros install them in
/usr/include/sys/...

I tried using relative paths and make the compiler find them, but apparently it doesn't work that way. Since the paths are already absolute I guess that's how it's supposed to be, so this seems the best solution to me. If someone has a better suggestion, please let me know. This is verified on Fedora 23 and Ubuntu 15.10.

I'm new to clang, CMake and GitHub, so feel free to point out anything that could be improved.

set(GLIBC_INCLUDE_PATH "/usr/include")
set(arch_dir "x86_64-linux-gnu")
if (EXISTS "${GLIBC_INCLUDE_PATH}/${arch_dir}/sys")
set(GLIBC_ARCH_INCLUDE_PATH "${GLIBC_INCLUDE_PATH}/${arch_dir}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use two spaces for indentation, for spaces for line continuations.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gribozavr: How about adding an uncrustify config to the project that has your code formatting so that we run it as a git-hook before making pull requests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a bigger discussion that should be held on swift-dev@swift.org. Feel free to start it, but I don't think you should block your patch on that.

@gribozavr
Copy link
Contributor

I ran the tests on Ubuntu 14.04 and it looks good. Please make the formatting and style changes that I mentioned above, and I'll merge.

set(outputs)
# Set correct paths to glibc headers
set(GLIBC_INCLUDE_PATH "/usr/include")
set(arch_dir "x86_64-linux-gnu")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using CMAKE_LIBRARY_ARCHITECTURE here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, as a side note: At some point it there will probably be a better way to get this path: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=796545

@estan
Copy link

estan commented Dec 8, 2015

Can confirm that this makes the build work on Arch as well.

@insha
Copy link

insha commented Dec 8, 2015

I can confirm that this makes the build work on Fedora 23 as well.

@orobio orobio force-pushed the detect-location-of-glibc-headers branch from 1487d75 to 041c031 Compare December 8, 2015 07:40
@orobio
Copy link
Contributor Author

orobio commented Dec 8, 2015

Updated the commit:

  • Fixed indentation
  • Consistently quoting ${input}
  • Removed now unused custom command target 'copy_glibc_module'
  • Used CMAKE_LIBRARY_ARCHITECTURE as suggested by @estan

@estan
Copy link

estan commented Dec 8, 2015

Nice work. A question though: What happened to the creation of the output directory now? It seems it's gone. Is it created implicitly somehow, or have we all had the directory already created when we tested this?

Also, and this is just a suggestion, but couldn't the CMakeLists.txt now be simplified to something like (untested):

set(sources
  Glibc.swift
)

set(output_dir "${SWIFTLIB_DIR}/glibc")

file(MAKE_DIRECTORY "${output_dir}")

# Set correct paths to glibc headers
set(GLIBC_INCLUDE_PATH "/usr/include")
if(CMAKE_LIBRARY_ARCHITECTURE)
  set(GLIBC_ARCH_INCLUDE_PATH "${GLIBC_INCLUDE_PATH}/${CMAKE_LIBRARY_ARCHITECTURE}")
else()
  set(GLIBC_ARCH_INCLUDE_PATH "${GLIBC_INCLUDE_PATH}")
endif()
if (NOT EXISTS "${GLIBC_ARCH_INCLUDE_PATH}/sys")
  message(FATAL_ERROR "Glibc headers were not found.")
endif()

configure_file(module.map.in "${output_dir}/module.map" @ONLY)

swift_install_in_component(stdlib
    FILES "${output_dir}/module.map"
    DESTINATION "lib/swift/glibc")

add_swift_library(swiftGlibc IS_SDK_OVERLAY
    ${sources}
    FILE_DEPENDS "${output_dir}/module.map" "${SWIFTLIB_DIR}/glibc"
    INSTALL_IN_COMPONENT stdlib-experimental)

What I did was:

  • Add file(MAKE_DIRECTORY "${output_dir}")
  • Let sources be the list of Swift sources
  • Handle module.map.in / module.map as a special case (not likely there will be many?), so no loop / regex substitutions needed.

I'll let someone from the Swift project comment on what they think of that.

@estan
Copy link

estan commented Dec 8, 2015

I also realized now that with the custom command target gone, I get

CMake Warning (dev):
  Policy CMP0058 is not set: Ninja requires custom command byproducts to be
  explicit.  Run "cmake --help-policy CMP0058" for policy details.  Use the
  cmake_policy command to set the policy and suppress this warning.

  This project specifies custom command DEPENDS on files in the build tree
  that are not specified as the OUTPUT or BYPRODUCTS of any
  add_custom_command or add_custom_target:

   lib/swift/glibc
   lib/swift/glibc/module.map

  For compatibility with versions of CMake that did not have the BYPRODUCTS
  option, CMake is generating phony rules for such files to convince 'ninja'
  to build.

  Project authors should add the missing BYPRODUCTS or OUTPUT options to the
  custom commands that produce these files.
This warning is for project developers.  Use -Wno-dev to suppress it.

with CMake 3.4.0. The custom command DEPENDS it speaks of is that added by add_swift_library. Not sure what the best way to handle this is, or if it should just be ignored for now. The cmake --help-policy CMP0058 is quite lengthy...

Could always do cmake_policy(SET CMP0058 OLD) in the top-level CMakeLists.txt to suppress it on CMake 3.4+. But I think that could be done in a separate pull request, if at all.

@orobio
Copy link
Contributor Author

orobio commented Dec 8, 2015

It's verified with a clean build that the directory is created, so that works fine.

I didn't want to rewrite too much of the file, because I'm new to CMake, but I agree that it can be simplified. And indeed, it might be best to have a look at that another time.

Regarding the warning, I didn't catch that, but just adding the custom command target back in seems pointless to me, since it's not really doing anything anymore. I would expect that it's possible to depend on a generated configuration file, but I can have another look at it tomorrow. In the meantime, if someone with more CMake experience has an easy solution, let me know.

@estan
Copy link

estan commented Dec 8, 2015

Alright. Good that the mkdir is not needed.

Yes, any possible simplifications can be another PR. Just wanted to throw it out there :)

I agree it's a little silly to have a "dummy" custom command. And the warning will only appear on CMake 3.4.0+, which are very new versions. So let's postpone that as well. I get truckloads of such policy warnings about other things anyway.

To me this can go in as is.

benizi added a commit to benizi/swift that referenced this pull request Dec 9, 2015
@urbaniak
Copy link

urbaniak commented Dec 9, 2015

👍

Was considering doing similar things.

Tested it on Fedora 23, works well.

In the Glibc module map all sys/... header files had the following paths:
/usr/include/x86_64-linux-gnu/sys/..., but some distros install them in
/usr/include/sys/...
@orobio orobio force-pushed the detect-location-of-glibc-headers branch from 041c031 to 3d91979 Compare December 9, 2015 12:40
@orobio
Copy link
Contributor Author

orobio commented Dec 9, 2015

Final update (hopefully). I'm becoming a CMake expert now, so I dared to rewrite a bit more ;)

  • Now creating module.map in the default location: ${CMAKE_CURRENT_BINARY_DIR}
  • Added custom command target to copy it to ${output_dir}
  • Cleaned up based on proposal by @estan

This seems to be the only solution to prevent the CMP0058 policy warning. I think normally you don't directly depend on generated configuration, but have an automatic dependency. For example with config.h. In this case we want to depend on module.map explicitly and it seems we need a custom command target for that.

There are other solutions, like generating a script for doing the substitutions, or generating a CMake file that is used during build, but I didn't want to move away too far from the working solution. If required, we can improve later.

Only drawback I see is the extra file we create, furthermore this solution works fine:

  • When just touching module.map.in, module.map is regenerated, but unchanged, so it's not copied to ${output_dir}.
  • When changing module.map.in, module.map is regenerated and copied, so it will trigger a rebuild.

@dowobeha
Copy link

dowobeha commented Dec 9, 2015

Thanks for this. I'd like to test this fix on Scientific Linux 7.1, but I'm having trouble applying the patch. I replaced stdlib/public/Glibc/CMakeLists.txt with the version from this commit, and likewise removed stdlib/public/Glibc/module.map and added the stdlib/public/Glibc/module.map.in from this commit.

But after doing so, the build doesn't know how to create stdlib/public/Glibc/module.map:

  • /home/lanes/cmake-3.4.1-Linux-x86_64.binary_distribution/bin/cmake --build /home/lanes/swift/build/Ninja-DebugAssert/swift-linux-x86_64 -- all swift-stdlib-linux-x86_64
    ninja: error: '/home/lanes/swift/swift/stdlib/public/Glibc/module.map', needed by 'lib/swift/glibc', missing and no known rule to make it

How should this fix be applied? Sorry for such a simple question - I'm fairly new to cmake and don't know ninja at all.

@orobio
Copy link
Contributor Author

orobio commented Dec 9, 2015

The changed build configuration should be detected. Did you try to run the build-script, or are you running cmake manually?

@dowobeha
Copy link

dowobeha commented Dec 9, 2015

I forgot to switch to the appropriate branch. Once I did that, I was able to build successfully on Scientific Linux 7.1

@orobio
Copy link
Contributor Author

orobio commented Dec 9, 2015

Ok 👍

@gribozavr
Copy link
Contributor

I tested on Ubuntu 14.04 and it works.

gribozavr added a commit that referenced this pull request Dec 10, 2015
Glibc module: detect location of glibc header files
@gribozavr gribozavr merged commit 5cf8e8e into swiftlang:master Dec 10, 2015
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
…s-tests

test suite fixes to prep for enabling additional warnings
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
DougGregor pushed a commit to DougGregor/swift that referenced this pull request Apr 28, 2024
…file

[cxx-interop] Add `usr/include/module.modulemap` file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants